Skip to content

Conversation

@greenc-FNAL
Copy link
Contributor

@greenc-FNAL greenc-FNAL commented Oct 30, 2025

  1. Dev container may be used outside of VSCode context.
  2. Project may be developed in "folder mode" rather than "workspace mode."
  3. Project may be developed in workspace mode without devcontainer.

This PR should not be disruptive to other development paradigms. If disruption is observed, please review and report.

@greenc-FNAL greenc-FNAL force-pushed the feature/vscode-dev-container branch 20 times, most recently from 39fe997 to 7979b32 Compare November 10, 2025 18:48
@greenc-FNAL greenc-FNAL force-pushed the feature/vscode-dev-container branch from 7979b32 to 9f8779a Compare January 6, 2026 00:31
@greenc-FNAL greenc-FNAL marked this pull request as ready for review January 6, 2026 21:48
Copilot AI review requested due to automatic review settings January 6, 2026 21:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces VSCode development container support and workspace configuration for the Phlex project, enabling developers to work in a consistent, containerized environment with pre-configured tooling and settings.

Key changes:

  • Adds VSCode workspace file with language-specific settings and extension recommendations
  • Creates devcontainer configuration with comprehensive development tooling setup
  • Refactors CI Dockerfile to separate user creation from base image and move developer-specific tooling to dev target

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
phlex.code-workspace VSCode workspace configuration with C++/Python settings, file associations, and extension recommendations
.devcontainer/devcontainer.json Dev container configuration specifying build settings, mounts, environment variables, and pre-installed extensions
.devcontainer/Dockerfile Creates vscode user and adds to spack group on top of phlex-dev base image
.devcontainer/cmake_wrapper.sh Wrapper script to source entrypoint.sh before running cmake
.devcontainer/ctest_wrapper.sh Wrapper script to source entrypoint.sh before running ctest
ci/Dockerfile Refactors user creation out of dev target, moves py-cmake-format and ruff installation to dev-only stage, reorganizes Spack repository configuration

# Create the user and add to spack group
RUN groupadd --gid $USER_GID $USERNAME \
&& useradd --uid $USER_UID --gid $USER_GID --create-home $USERNAME \
&& usermod -aG $SPACK_GID $USERNAME \
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SPACK_GID is passed as an argument but then used directly as a numeric GID in the usermod command. The variable should be used to reference the spack group name, not passed as a numeric GID. The correct usage would be to use the group name 'spack' or reference the actual SPACK_GROUP environment variable from the base image.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI commented Jan 6, 2026

@greenc-FNAL I've opened a new pull request, #223, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Jan 7, 2026

@greenc-FNAL I've opened a new pull request, #224, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Jan 7, 2026

@greenc-FNAL I've opened a new pull request, #226, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

chmod 0440 /etc/sudoers.d/$USERNAME
usermod -aG $SPACK_GROUP $USERNAME
CREATE_DEV_USER
# (User creation removed to be handled by devcontainer or runtime environment)
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "User creation removed to be handled by devcontainer or runtime environment" but this is misleading. The user creation logic wasn't removed - it was moved to .devcontainer/Dockerfile. Consider updating this comment to say "User creation moved to .devcontainer/Dockerfile to support flexible runtime environments" to accurately reflect what happened.

Suggested change
# (User creation removed to be handled by devcontainer or runtime environment)
# User creation moved to .devcontainer/Dockerfile to support flexible runtime environments

Copilot uses AI. Check for mistakes.
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI commented Jan 7, 2026

@greenc-FNAL I've opened a new pull request, #227, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Jan 7, 2026

@greenc-FNAL I've opened a new pull request, #228, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI added 2 commits January 7, 2026 12:05
A code review bot flagged .devcontainer/ctest_wrapper.sh and .devcontainer/cmake_wrapper.sh as potentially missing executable permissions. Investigation confirmed both files have correct permissions (mode 100755) tracked in Git since commit 7401741.

No changes required - the wrapper scripts are properly executable.

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Addressed review comment questioning whether .devcontainer/cmake_wrapper.sh and .devcontainer/ctest_wrapper.sh have proper executable permissions.

Investigation Results
Both wrapper scripts have been tracked by Git with mode 100755 (executable) since creation in commit 5c77ab9
Filesystem permissions confirm executable status (-rwxrwxr-x)
No changes required; original concern was incorrect
The wrapper scripts are properly configured and functioning as intended.

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI commented Jan 7, 2026

@greenc-FNAL I've opened a new pull request, #229, to work on those changes. Once the pull request is ready, I'll request review from you.

Per [review feedback](#87 (comment)), the devcontainer referenced a hardcoded Python version path (`python3.14/site-packages`) that requires manual updates when the Spack-installed Python version changes.

## Changes

- **`.devcontainer/Dockerfile`**: Added validation step that ensures exactly one versioned Python directory exists in the Spack view, creates/updates `/opt/spack-environments/phlex-ci/.spack-env/view/lib/python` symlink to point to it, and fails the build if zero or multiple versioned directories are found

- **`.devcontainer/devcontainer.json`**: Changed `python.analysis.extraPaths` from versioned path to symlink path

## Example

The validation script runs during image build:

```bash
# Finds python3.14/ (or whatever version Spack installed)
python_dirs=("$SPACK_VIEW_LIB"/python3.*)

# Errors if count != 1
if [ $count -eq 0 ]; then
  echo "ERROR: No versioned Python site-packages directory found" >&2
  exit 1
fi

# Creates lib/python -> lib/python3.14
ln -sn "$python_basename" "$python_link"
```

VSCode configuration now references the stable symlink path instead of the versioned directory.

---
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants